feat: add active filter count badge to UniversalFilter - #1753
Conversation
Wraps the filter TooltipIcon in a MUI Badge that displays the count of active (non-'All') filter values. Badge is invisible when no filters are active via MUI's native invisible prop. Fixes layer5io#1728 Signed-off-by: Udaybir Singh <writetoudaybir@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughUniversalFilter now counts active selected filters and displays that count in a Badge around the filter trigger. The badge is hidden when no non-default filters are active, and tests mock Badge output for DOM assertions. ChangesUniversalFilter active count badge
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/custom/UniversalFilter.tsx (1)
186-198: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a Sistent semantic token for the badge color.
color="primary"relies on MUI’s default color API rather than the project’s theme semantics. Style the badge with the appropriate Sistent semantic palette token so its applied-filter state remains theme-consistent.As per coding guidelines, “Theme-aware UI must use Sistent theme exports and semantic palette tokens rather than raw MUI defaults.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/custom/UniversalFilter.tsx` around lines 186 - 198, Update the Badge in the UniversalFilter component to replace the raw MUI color="primary" value with the appropriate Sistent semantic palette token from the theme, preserving the existing activeFilterCount visibility behavior.Source: Coding guidelines
src/__testing__/UniversalFilter.test.tsx (1)
115-188: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest badge updates after filters are applied and cleared.
These cases mount independent initial states only. Add a controlled-parent or
rerendertest that changesselectedFiltersfrom active toAll(and vice versa) to verify the badge updates during the user flow required by the issue.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/__testing__/UniversalFilter.test.tsx` around lines 115 - 188, Extend the badge tests around UniversalFilter with a rerender or controlled-parent flow that changes selectedFilters between an active value and 'All'. Assert that filter-badge data-content and data-invisible update correctly in both directions, preserving the existing initial-state coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/__testing__/UniversalFilter.test.tsx`:
- Around line 58-65: Update the Badge mock component in UniversalFilter.test.tsx
to replace the any props annotation with an explicit React prop type covering
optional badgeContent, optional invisible, and children; preserve the mock’s
existing rendering and data attributes.
---
Nitpick comments:
In `@src/__testing__/UniversalFilter.test.tsx`:
- Around line 115-188: Extend the badge tests around UniversalFilter with a
rerender or controlled-parent flow that changes selectedFilters between an
active value and 'All'. Assert that filter-badge data-content and data-invisible
update correctly in both directions, preserving the existing initial-state
coverage.
In `@src/custom/UniversalFilter.tsx`:
- Around line 186-198: Update the Badge in the UniversalFilter component to
replace the raw MUI color="primary" value with the appropriate Sistent semantic
palette token from the theme, preserving the existing activeFilterCount
visibility behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0df0062b-85a5-4b55-95d8-89bc12b89cb2
📒 Files selected for processing (2)
src/__testing__/UniversalFilter.test.tsxsrc/custom/UniversalFilter.tsx
| jest.mock('../base/Badge', () => ({ | ||
| Badge: ({ children, badgeContent, invisible }: any) => ( | ||
| <div data-testid="filter-badge" data-content={badgeContent} data-invisible={String(invisible)}> | ||
| {children} | ||
| </div> | ||
| ) | ||
| })); | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files src/__testing__/UniversalFilter.test.tsx
echo '---'
cat -n src/__testing__/UniversalFilter.test.tsx | sed -n '1,260p'Repository: layer5io/sistent
Length of output: 7096
Type the Badge mock props explicitly. Replace the any here with a small prop type, such as React.PropsWithChildren<{ badgeContent?: React.ReactNode; invisible?: boolean }>.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/__testing__/UniversalFilter.test.tsx` around lines 58 - 65, Update the
Badge mock component in UniversalFilter.test.tsx to replace the any props
annotation with an explicit React prop type covering optional badgeContent,
optional invisible, and children; preserve the mock’s existing rendering and
data attributes.
Source: Coding guidelines
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/custom/UniversalFilter.tsx (1)
244-251: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssociate the quick-range label with the Select.
InputLabelis detached from theSelect, so screen readers may announce the control without its “Quick Ranges” name. AddlabelId={${testId}-quick-range-label}to theSelect.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/custom/UniversalFilter.tsx` around lines 244 - 251, Update the Select associated with the quick-range InputLabel to include the matching labelId value `${testId}-quick-range-label`, ensuring the control is programmatically named “Quick Ranges.”src/__testing__/publishedTypeSurfaceDependencies.test.ts (3)
95-97: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDetect side-effect imports in declaration bundles.
specifiersInmissesimport 'side-effect-package';. A package referenced only this way is still required by consumers, but the current detector returns no reference and can let the dependency-contract test pass incorrectly.Extend the matcher and add a detector fixture for side-effect imports.
Proposed fix
const specifiersIn = (source: string): string[] => [ - ...new Set([...source.matchAll(/(?:from|import\()\s*['"]([^'"]+)['"]/g)].map((match) => match[1])) + ...new Set( + [...source.matchAll(/\b(?:from|import(?:\s*\(\s*)?)\s*['"]([^'"]+)['"]/g)].map( + (match) => match[1] + ) + ) ];Also applies to: 214-224
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/__testing__/publishedTypeSurfaceDependencies.test.ts` around lines 95 - 97, Update specifiersIn to recognize bare side-effect imports such as import 'package' in addition to from and dynamic import() forms, while preserving extraction of the module specifier. Add a detector fixture covering a declaration bundle containing only a side-effect import and assert that the referenced package is detected by the dependency-contract test.
140-181: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftValidate the published surface with a scoped
tscfixture.This test only scans declaration text and package metadata; it never type-checks a real consumer. Add a scoped fixture, invoke
tsc, verify the fixture appears in--listFiles, and filter diagnostics to that fixture so the test validates actual consumer resolution rather than only regex results.As per coding guidelines, type-contract tests under
src/__testing__/*.test.tsmust use this scopedtscverification approach.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/__testing__/publishedTypeSurfaceDependencies.test.ts` around lines 140 - 181, Extend the published type-surface test around the existing dist/index.d.ts checks with a scoped consumer fixture that imports the package and is type-checked via tsc. Verify the fixture is present in tsc’s --listFiles output, filter diagnostics to the fixture, and fail on relevant resolution or type errors while preserving the existing metadata and declaration-text assertions.Source: Coding guidelines
66-77: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestore the documented optional-peer exemption.
The comments state that
@mui/x-date-pickersis intentionally exempted whileDateTimePickerPropsremains publicly re-exported, butOPTIONAL_PEERS_ON_THE_RECORDcontains onlyreact. Because optional peers are removed from the consumer-guaranteed set, this causes the suite to fail whenever the declaration bundle references@mui/x-date-pickers.Either add the documented exemption until issue
#1749is fixed, or remove the public type reference before narrowing the list.Proposed fix
-const OPTIONAL_PEERS_ON_THE_RECORD = ['react']; +const OPTIONAL_PEERS_ON_THE_RECORD = ['`@mui/x-date-pickers`', 'react'];Based on learnings, derive optional-peer enforcement from
src/__testing__/optionalPeerDependencies.test.tsrather than narrowing the list independently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/__testing__/publishedTypeSurfaceDependencies.test.ts` around lines 66 - 77, Restore the documented `@mui/x-date-pickers` exemption in OPTIONAL_PEERS_ON_THE_RECORD so the published type-surface test remains consistent with the barrel’s DateTimePickerProps re-export, or remove that public type reference before narrowing the exemption list. Prefer reusing the optional-peer enforcement or exemption definition from optionalPeerDependencies.test.ts instead of maintaining a separate independently narrowed list.Source: Learnings
🧹 Nitpick comments (1)
src/custom/UniversalFilter.tsx (1)
290-294: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a Sistent semantic badge color.
color="primary"relies on the raw MUI color API. Use the project’s semantic Sistent palette/token styling for the badge so it remains consistent across themes.As per coding guidelines, “Theme-aware UI must use Sistent theme exports and semantic palette tokens rather than raw MUI defaults.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/custom/UniversalFilter.tsx` around lines 290 - 294, Update the Badge in UniversalFilter to replace the raw MUI color="primary" value with the project’s Sistent semantic badge color or theme palette token. Preserve the existing activeFilterCount, overlap, and visibility behavior while using the established theme-aware styling approach.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/__testing__/publishedTypeSurfaceDependencies.test.ts`:
- Around line 95-97: Update specifiersIn to recognize bare side-effect imports
such as import 'package' in addition to from and dynamic import() forms, while
preserving extraction of the module specifier. Add a detector fixture covering a
declaration bundle containing only a side-effect import and assert that the
referenced package is detected by the dependency-contract test.
- Around line 140-181: Extend the published type-surface test around the
existing dist/index.d.ts checks with a scoped consumer fixture that imports the
package and is type-checked via tsc. Verify the fixture is present in tsc’s
--listFiles output, filter diagnostics to the fixture, and fail on relevant
resolution or type errors while preserving the existing metadata and
declaration-text assertions.
- Around line 66-77: Restore the documented `@mui/x-date-pickers` exemption in
OPTIONAL_PEERS_ON_THE_RECORD so the published type-surface test remains
consistent with the barrel’s DateTimePickerProps re-export, or remove that
public type reference before narrowing the exemption list. Prefer reusing the
optional-peer enforcement or exemption definition from
optionalPeerDependencies.test.ts instead of maintaining a separate independently
narrowed list.
In `@src/custom/UniversalFilter.tsx`:
- Around line 244-251: Update the Select associated with the quick-range
InputLabel to include the matching labelId value `${testId}-quick-range-label`,
ensuring the control is programmatically named “Quick Ranges.”
---
Nitpick comments:
In `@src/custom/UniversalFilter.tsx`:
- Around line 290-294: Update the Badge in UniversalFilter to replace the raw
MUI color="primary" value with the project’s Sistent semantic badge color or
theme palette token. Preserve the existing activeFilterCount, overlap, and
visibility behavior while using the established theme-aware styling approach.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cbd00669-0971-4d97-b83f-8939ac126815
📒 Files selected for processing (3)
src/__testing__/UniversalFilter.test.tsxsrc/__testing__/publishedTypeSurfaceDependencies.test.tssrc/custom/UniversalFilter.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/testing/UniversalFilter.test.tsx
82f39f3 to
33880e5
Compare
Signed-off-by: Udaybir Singh <writetoudaybir@gmail.com>
|
@KhushamBansal ready for review!! |
KhushamBansal
left a comment
KhushamBansal
left a comment
There was a problem hiding this comment.
Tested it in meshery-ui, LGTM!!
|
@rishiraj38 @YASHMAHAKAL @leecalcote |
|
Could you show an use case for this? Currently, I think filters aren't something which you can multiple off in a request, but rather a one value item. So, this is pretty much serves more as a conditional rather than an aggregator as to how many items are being selected. It'll always say one and if that's the case, then it should probably streamlined to that use case |
|
@banana-three-join Lee requested this via Khusham , happy to loop them in if you want to clarify the use case. |
|
Yup, @leecalcote mentioned during one of our meetings that we wanted to add a badge indicating the number of filters applied. |
Notes for Reviewers
This PR fixes #1728
Signed commits
Changes
src/custom/UniversalFilter.tsxsrc/__testing__/UniversalFilter.test.tsxVerification
Summary by CodeRabbit
New Features
Tests